-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: select component a11y changes #1066
Conversation
Deploy preview for fundamental-styles ready! Built with commit d8b53cf |
docs/pages/components/select.md
Outdated
<div class="fd-select__control" role="button" aria-expanded="false" aria-haspopup="false" aria-readonly="true" readonly> | ||
Select | ||
</div> | ||
<div class="fd-select__control" role="button" aria-expanded="false" aria-label="Select" aria-haspopup="false" aria-readonly="true" readonly></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have an aria-label like I've added here because the placeholder is removed in readOnly state? @meganaconley @jacobdevera @prsdthkr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show where it says not to include a placeholder? I'm looking at https://experience.sap.com/fiori-design-web/select/#readonly and it seems fine as-is.
I think the confusion may be that it seems to be a placeholder when it should actually be a pre-selected value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For everyone else - sent wiki link privately as it's internal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can set this one to be a selected item for the docs though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the text here could be changed to make clear that it's a selected item rather than a placeholder.
package.json
Outdated
@@ -123,8 +123,7 @@ | |||
"pre-commit": { | |||
"colors": true, | |||
"run": [ | |||
"lint:pre-commit", | |||
"storybook:visual-docker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be running storybook on a pre-commit hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It already was - otherwise you wouldn't be able to commit anything 😉
#1067
Deploy preview for fundamental-styles ready! Built with commit 3fd59fc |
docs/pages/components/select.md
Outdated
@@ -25,7 +25,7 @@ For lists that require more than 12 options, the <a href="/patterns/combobox-inp | |||
<div class="fd-select"> | |||
<div class="fd-select__control" role="button" tabindex="0" aria-controls="h0C6A325" aria-expanded="false" aria-haspopup="true"> | |||
Select | |||
<span class="fd-button fd-button--transparent sap-icon--slim-arrow-down fd-select__button"></span> | |||
<button class="fd-button fd-button--transparent sap-icon--slim-arrow-down fd-select__button"></button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need any aria-hidden attributes? aria-label? or does the placeholder give enough information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should put a tabIndex
of -1 on this. Otherwise we get two tab stops. The entire select should be a single tabstop as in this example: https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.html
That way it can be a button or a span, whichever is better for the visuals.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure about the change to a button
@@ -25,7 +25,7 @@ For lists that require more than 12 options, the <a href="/patterns/combobox-inp | |||
<div class="fd-select"> | |||
<div class="fd-select__control" role="button" tabindex="0" aria-controls="h0C6A325" aria-expanded="false" aria-haspopup="true"> | |||
Select | |||
<span class="fd-button fd-button--transparent sap-icon--slim-arrow-down fd-select__button"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to a span
to fix the duplicate tabbing issue (you can observe this now by tabbing into the Select component in this PR's deployment). I guess you could make an argument that anything with the fd-button
class should be an actual button
, but the parent being role="button"
is sufficient for screen readers and keyboard users to use the component without majorly refactoring the styles.
docs/pages/components/select.md
Outdated
<div class="fd-select__control" role="button" aria-expanded="false" aria-haspopup="false" aria-readonly="true" readonly> | ||
Select | ||
</div> | ||
<div class="fd-select__control" role="button" aria-expanded="false" aria-label="Select" aria-haspopup="false" aria-readonly="true" readonly></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you show where it says not to include a placeholder? I'm looking at https://experience.sap.com/fiori-design-web/select/#readonly and it seems fine as-is.
I think the confusion may be that it seems to be a placeholder when it should actually be a pre-selected value.
docs/pages/components/select.md
Outdated
<div class="fd-select__control" role="button" aria-expanded="false" aria-haspopup="false" aria-readonly="true" readonly> | ||
Select | ||
</div> | ||
<div class="fd-select__control" role="button" aria-expanded="false" aria-label="Select" aria-haspopup="false" aria-readonly="true"></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have tabindex="0"
since readonly inputs are focusable for screen-reader purposes. Also I believe you removed the selected value by mistake.
<div class="fd-select__control" role="button" aria-expanded="false" aria-label="Potato" aria-haspopup="false" aria-readonly="true" tabindex="0">Potato</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion. OTTLGTM ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package.json
Outdated
@@ -123,8 +123,7 @@ | |||
"pre-commit": { | |||
"colors": true, | |||
"run": [ | |||
"lint:pre-commit", | |||
"storybook:visual-docker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to separate PR
@jacobdevera @prsdthkr @salarenko @meganaconley I made a lot of new changes - see new edited description of the pr please! I highly recommend looking at the storybook pages instead of the docs site to fully see everything |
<li class="fd-list__item" role="option" tabindex="-1"> | ||
<li class="fd-list__item fd-select__item" | ||
role="option" | ||
tabindex="-1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing changed in these besides the class on fd-list__item- I was just spreading them out so I could see what I was doing. Happy to put them back if anyone is bothered by it
<div class="fd-select__control" role="button" aria-expanded="false" aria-haspopup="false" aria-readonly="true" readonly> | ||
Select | ||
</div> | ||
<div class="fd-select__control is-readonly" role="button" aria-expanded="false" aria-label="Select" aria-haspopup="false">Selected Item 2</div> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where I still don't know what to do.. I've got the styling added via is-readonly
class, but how do we communicate it's readonly? The ui5 one has a hidden input.
Additionally, I'm not sure how about not being able to open the popover - "Readonly elements are relevant to the user, and application authors SHOULD NOT restrict navigation to the element or its focusable descendants." https://www.w3.org/TR/2017/REC-wai-aria-1.1-20171214/#aria-readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been trying to look for other examples on the internet, and there doesn't appear to be anything like this surprisingly. I agree the language is a little vague on what to do for components with popups. We have a couple steps:
-
Like UI5, we can add a hiddeninput
witharia-readonly
as a child of the button. This is so screen readers can tell it is read-only when focused without have to click the button. -
(optional?) Allow the popover to open in read only mode, and set
aria-readonly
on theul
listbox
.
We should definitely do 1. 2 is potentially optional. The disappearance of the chevron in readonly suggests that it shouldn't be opened, and I can't find any mandate where the list of options needs to be presented in read-only mode.
EDIT: I was experimenting putting an inner input, but the readonly state is not read out when the button is focused. The inner input doesn't appear to help much at all. If I change the button itself to a role="combobox"
with aria-readonly
this reads as expected. The problem is that a Select effectively always has a readonly combobox as you can't edit the text directly, which further muddies the problem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to say that a combobox is aria-readonly
only when it's value can't be set. Technically you can't type in the box, but you can change its value. The definition in the link Jenna provided for aria-readonly
says The user can set the value of the element.
vs The user cannot change the value of the element.
By that I don't think it's limited to actually typing only, but any sort of change in value whether it be selecting an option from a dropdown or a radio group, setting a checkbox, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meganaconley @jacobdevera Stupid question - but I'm not following 😆 What should the html look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbadan Yeah sorry, I was thinking out loud and got kinda convoluted 😅
Like UI5 (https://sapui5.hana.ondemand.com/#/entity/sap.m.Select/sample/sap.m.sample.Select), I would try to change to combobox
instead of button
because it supports aria-readonly
:
<div class="fd-select__control is-readonly" role="combobox" aria-expanded="false" aria-label="Select" aria-haspopup="false" aria-readonly="true">Selected Item 2</div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is what I was running into: https://www.w3.org/TR/wai-aria-1.1/#combobox
"Authors MUST ensure an element with role combobox contains or owns a text input element with role textbox or searchbox and that the text input has aria-multiline set to false. "
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, we are already breaking spec because we are not supplying an editable textbox input for the combobox in the first place; we're merely using combobox as a way to get aria-readonly
to be supported. The spec mentions that the input
must be a child of the combobox, not that the input can the combobox itself. In our case, if we supply the fake hidden input element like UI5 does, at best, it doesn't affect anything for screen reader callouts, and at worst, could be misleading for when the Select is not marked readonly.
Again, whether the base combobox is an input
or div
doesn't affect accessibility because the semantics are being overriden by the combobox role.
My take: I'm okay with adding a hidden input into a combobox, but only for the readonly state. All other select states should remain with the usual listbox in button
as in the w3c example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what I'm trying to say - should we even be using role="combobox"
if we're not following the spec?
This is starting to feel like too hacky of a workaround. We can also just table this and put an actual story in the backlog to investigate this further since we've spent so much time on it already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, no. Agreed that it can wait for a future story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UIA11Y-2324
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jbadan good job with A11y improvements. Let's take a look at my comments
src/select.scss
Outdated
&__item[aria-selected=true] { | ||
border-bottom: 0 !important; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need to have fd-select__item
on every single item anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which spec do we want to follow as the source of truth? https://experience.sap.com/fiori-design-web/select/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only assume, that we should follow list component there. Same as with multi input or combobox. Let's ask designers maybe, because spec shows 2 different views.
<ul class="fd-list fd-list--has-message fd-list--dropdown fd-list--compact" role="listbox"> | ||
<div aria-live="assertive" class="fd-list__message fd-list__message--error" role="alert">Error message</div> | ||
<li class="fd-list__item is-selected" aria-selected="true" role="option" tabindex="0"> | ||
<div aria-live="assertive" class="fd-list__message fd-list__message--error" role="alert">Error message</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jacobdevera @salarenko @JKMarkowski updated for pr feedback. Note that the readonly state will be handled in a separate followup pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/mixins/_forms.scss
Outdated
@@ -85,6 +85,7 @@ | |||
} | |||
|
|||
&[readonly], | |||
&[aria-readonly="true"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use @mixin fd-readonly
@salarenko updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
Related Issue
Closes SAP/fundamental-styles#
Description
The design spec for readOnly:
Placeholder text and Button are both NOT displayed in read only mode.
Added min-width:
Min-Width (Select field version with text): 5 rem to be able to see any text and the icon
Breaking changes:
aria-labeledby
tofd-list ul
connect toid
onfd-form-label
, see https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.htmldiv fd-list__message
outside offd-list ul
-span
cannot be a descendent oful
fd-popover__control
as this is adiv
and cannot have adisabled
attributedisabled
attribute in favor ofaria-disabled=“true”
: When the action associated with a button is unavailable, the button has aria-disabled set to true. https://www.w3.org/TR/wai-aria-practices/#wai-aria-roles-states-and-properties-3tabIndex=“-1” to
fd-list ul`: https://www.w3.org/TR/wai-aria-practices/examples/listbox/listbox-collapsible.htmlaria-readonly
is not allowed onrole=“button”
, replaced with “is-readonly” class or readonly attribute https://www.w3.org/TR/2017/REC-wai-aria-1.1-20171214/#aria-readonlyAdditional comments:
Note there is a deleted test in here - part of #1108